Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Sync client-go – third try #40732

Merged
merged 4 commits into from
Jan 31, 2017
Merged

Sync client-go – third try #40732

merged 4 commits into from
Jan 31, 2017

Conversation

sttts
Copy link
Contributor

@sttts sttts commented Jan 31, 2017 •
edited

Follow-up of #40692 & #40699.

Approval and lgtm from #40692.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jan 31, 2017
@k8s-github-robot k8s-github-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. release-note-label-needed labels Jan 31, 2017
@k8s-github-robot
Copy link

[APPROVALNOTIFIER] This PR is NOT APPROVED

The following people have approved this PR: sttts

Needs approval from an approver in each of these OWNERS Files:

We suggest the following people:
cc @thockin
You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

@k8s-reviewable
Copy link

This change is Reviewable

@sttts sttts added release-note-none Denotes a PR that doesn't merit a release note. approved Indicates a PR has been approved by an approver from all required OWNERS files. and removed release-note-label-needed labels Jan 31, 2017
deads2k
deads2k reviewed Jan 31, 2017
View reviewed changes
hack/make-rules/verify.sh
@@ -23,10 +23,9 @@ source "${KUBE_ROOT}/cluster/lib/util.sh"

# Excluded checks are always skipped.
EXCLUDED_CHECKS=(
"verify-linkcheck.sh" # runs in separate Jenkins job once per day due to high network usage
"verify-govet.sh" # it has a separate make vet target
"verify-staging-client-go.sh" # TODO: enable the script after 1.5 code freeze
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thsi has to stay excluded. cycle

@deads2k
Copy link
Contributor

deads2k commented Jan 31, 2017

client-go: initial examples import - the intent is good, but my guess is that this will make a mess of whatever they're using to sync.

deads2k
deads2k reviewed Jan 31, 2017
View reviewed changes
hack/verify-staging-client-go.sh
@@ -72,3 +72,16 @@ godep restore ${V} 2>&1 | sed 's/^/ /'

echo "Testing staging/copy.sh"
staging/copy.sh -d 2>&1 | sed 's/^/ /'

# Check that the apimachinery Godeps revision is actually in Github
echo "Checking that a valid k8s.io/apimachinery is reference in client-go"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unnecessary. This is always the case unless you do something insane and manual to your godeps.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's the point: due to the cycle during development we will do insane things.

Copy link
Contributor Author

@sttts sttts Jan 31, 2017 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The idea: hack/update-client-go.sh -s.

This gives you a client which has no valid apimachinery upstream rev, but works great for WIP PRs.

deads2k
deads2k reviewed Jan 31, 2017
View reviewed changes
hack/update-client-go.sh

# Create a temporary GOPATH for apimachinery and client-go, copy the current master into each and turn them
# into a git repo. Then we can run copy.sh with this additional GOPATH. The Godeps.json will
# have invalid git SHA1s, but it's good enough as a smoke test of copy.sh.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why would we do this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To make sure copy.sh is working?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how do you run godep without a real repo?

@deads2k deads2k added approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm "Looks good to me", indicates that a PR is ready to be merged. and removed approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jan 31, 2017
@deads2k
Copy link
Contributor

deads2k commented Jan 31, 2017

@k8s-bot kubemark e2e test this

@k8s-github-robot
Copy link

Automatic merge from submit-queue (batch tested with PRs 38772, 38797, 40732, 40740)

@k8s-github-robot k8s-github-robot merged commit e40da1e into kubernetes:master Jan 31, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@deads2k deads2k deads2k left review comments

Assignees

@deads2k deads2k

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note-none Denotes a PR that doesn't merit a release note. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@sttts @k8s-github-robot @k8s-reviewable @deads2k @k8s-ci-robot